Code Review

This PR is a well-executed semver hotfix — restoring accidentally-deleted public APIs with [Obsolete] attributes is exactly the right approach before a major bump. The overall strategy and execution are sound.

Issues Found

1. Duplicated count-extraction logic in CountWrapper (minor, but worth a note)

The following pattern is copy-pasted across 6 separate methods in CountWrapper.cs:

if (value == null) return 0;
if (value is ICollection collection) return collection.Count;
return value.Cast<object>().Count();

Even for [Obsolete] code that will be deleted at v2, this creates 6 separate maintenance points until deletion and adds ~60 lines of noise. A single private helper removes all that:

private static int GetCount(TCollection? value) =>
    value is null ? 0
    : value is ICollection c ? c.Count
    : value.Cast<object>().Count();

Then every method becomes a one-liner that maps to it. This is especially worth doing since this code will live in the codebase for potentially months before v2.

2. Thread-safety story around Timings / _timingsLock needs clarification

The PR adds _timingsLock to protect user-facing RecordTiming calls, and the PR description states "engine no longer writes to this list". However, the existing comment directly above the field says the opposite:

// Timings are written sequentially by the framework during test execution, never by user code.
internal List<TimingEntry> Timings { get; } = [];
private readonly Lock _timingsLock = new();

These two statements directly contradict each other. If the engine truly stopped writing to Timings, the old comment is stale and should be removed — if it's still written to by the engine, then _timingsLock alone doesn't protect against races between engine writes (unguarded) and user RecordTiming calls (guarded). Either way, the comment and the PR description can't both be right; please resolve that inconsistency to avoid confusion for future readers.

3. LengthWrapper restores only EqualTo, unlike CountWrapper

CountWrapper restores a full API surface (9 methods: EqualTo, GreaterThan, GreaterThanOrEqualTo, LessThan, LessThanOrEqualTo, Between, Zero, NotEqualTo, Positive), but LengthWrapper only restores EqualTo. If any v1.x user was calling HasLength().GreaterThan(5) they'll still get a compile error after upgrading, defeating the purpose of the fix.

If the original LengthWrapper had only EqualTo, this is fine — but it's worth confirming against the v1.26 git history to make sure the restore is complete and consistent.

What's Working Well

• [Obsolete] messages are clear and actionable — they tell users exactly what to use instead.
• The explicit interface implementation for ITestOutput.Timings / RecordTiming is the right approach; it avoids polluting TestContext's public surface with deprecated members.
• #pragma warning disable CS0618 is correctly scoped to only the bridge implementation.
• ObjectBag => StateBag aliasing is minimal and correct.
• PublicAPI snapshots are updated for all four targets (net8/9/10/472) — good hygiene.
• Tracking the actual deletion in #5604 is the right way to ensure these don't linger past v2.

Overall this is ready to merge pending the comment stale-ness fix and confirmation that LengthWrapper is fully restored.

Code Review: Restore [Obsolete] members removed in v1.27 (#5539)

This is a well-executed semver-fix PR. The approach is correct — restore with [Obsolete] rather than hard-delete in a minor release — and the implementation is clean.

What's Good

• Correct strategy: Restoring with [Obsolete] + clear migration messages ("Use Count().IsEqualTo(expectedCount) instead.") gives users a smooth upgrade path to v2.
• Feedback addressed: The refactor commit extracted GetCount/MapToCount helpers in CountWrapper to eliminate the 6x duplication that existed in the earlier revision. Good.
• <inheritdoc cref="StateBag"/> on the ObjectBag aliases avoids doc duplication and will stay in sync automatically.
• Lock justification is clear: The comment in TestContext.Output.cs explains exactly why _timingsLock is needed (user-facing RecordTiming may be concurrent) and why engine writes don't need it (sequential, lifecycle-ordered), and the description reinforces that the engine no longer writes to this list at all.
• PublicAPI snapshots are updated consistently across all four TFMs.

Minor Observations

GetCount returns 0 for null
CountWrapper.GetCount silently maps a null collection to count 0. This means Assert.That((List<int>?)null).HasCount(0) would pass rather than fail/throw. Since this matches the original pre-deletion behavior and the API is obsolete, this is acceptable — but worth noting in case the original also had this quirk.

LengthWrapper only exposes EqualTo
CountWrapper exposes 8 assertion methods (GreaterThan, LessThan, Between, Zero, etc.) while LengthWrapper only exposes EqualTo. If this is a faithful restoration of the original API, it's correct — just worth confirming that HasLength().GreaterThan(...) was never supported, to avoid silently dropping functionality.

CountWrapper throws NotSupportedException at runtime for IAssertionSource<TCollection> interface methods
Methods like IsTypeOf<TExpected>() throw at runtime rather than failing at compile time. This is unavoidable given the interface contract, and the error messages are excellent ("Use: Assert.That(value).IsTypeOf<List>().HasCount()..."). No change needed — just noting it as a design consequence.

Conclusion

The PR correctly fixes the semver regression from #5384 and the implementation is solid. Previous review feedback has been addressed. Ready to merge.

Code Review: Restore [Obsolete] members removed in v1.27 (#5539)

This is a well-executed semver-fix PR. The approach is correct — restore with [Obsolete] rather than hard-delete in a minor release — and the implementation is clean.

What's Good

• Correct strategy: Restoring with [Obsolete] + clear migration messages ("Use Count().IsEqualTo(expectedCount) instead.") gives users a smooth upgrade path to v2.
• Feedback addressed: The refactor commit extracted GetCount/MapToCount helpers in CountWrapper to eliminate the 6x duplication that existed in the earlier revision.
• <inheritdoc cref="StateBag"/> on the ObjectBag aliases avoids doc duplication and will stay in sync automatically.
• Lock justification is clear: The comment in TestContext.Output.cs explains exactly why _timingsLock is needed (user-facing RecordTiming may be concurrent) and why engine writes don't need it (sequential, lifecycle-ordered, no longer writing to this list).
• PublicAPI snapshots are updated consistently across all four TFMs.

Minor Observations

GetCount returns 0 for null: CountWrapper.GetCount silently maps a null collection to count 0. Since this matches the original pre-deletion behavior and the API is obsolete, this is acceptable — worth noting in case the original also had this quirk.

LengthWrapper only exposes EqualTo while CountWrapper exposes 8 assertion methods. If this faithfully restores the original API, it's correct. (Author confirmed this matches the original — HasLength().GreaterThan(...) was never supported.)

CountWrapper throws NotSupportedException at runtime for IAssertionSource<TCollection> interface methods like IsTypeOf. This is unavoidable given the interface contract, and the error messages are excellent with clear guidance.

Conclusion

The PR correctly fixes the semver regression from #5384. Implementation is solid, thread-safety is properly handled, and previous review feedback has been fully addressed. Ready to merge.